Conversation
The workflow's REGISTRY env var was overriding the Makefile's REGISTRY ?= default, causing images to be built with wrong names. Rename workflow env vars to PUSH_REGISTRY/PUSH_IMAGE, query the Makefile for actual built image names, and rename bink-node to node. Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reviewer's GuideUpdates the node image build workflow to avoid clobbering the Makefile’s REGISTRY defaults, align image names with the Makefile (renaming bink-node to node), and drive push tagging from Makefile-provided image names instead of hard-coded localhost/bink-node references. Sequence diagram for updated node image build and push workflowsequenceDiagram
actor Developer
participant GitHubActions
participant FedoraMakefile
participant ghcr_io
Developer->>GitHubActions: push node change
GitHubActions->>GitHubActions: set PUSH_REGISTRY, PUSH_IMAGE
GitHubActions->>FedoraMakefile: make print-bootc-image
FedoraMakefile-->>GitHubActions: BOOTC_IMAGE
GitHubActions->>FedoraMakefile: make print-node-image
FedoraMakefile-->>GitHubActions: NODE_IMAGE
GitHubActions->>ghcr_io: podman login ghcr.io
GitHubActions->>ghcr_io: podman tag BOOTC_IMAGE PUSH_REGISTRY/PUSH_IMAGE:TAG
GitHubActions->>ghcr_io: podman push PUSH_REGISTRY/PUSH_IMAGE:TAG
GitHubActions->>ghcr_io: podman tag BOOTC_IMAGE PUSH_REGISTRY/PUSH_IMAGE:latest
GitHubActions->>ghcr_io: podman push PUSH_REGISTRY/PUSH_IMAGE:latest
GitHubActions->>ghcr_io: podman tag NODE_IMAGE PUSH_REGISTRY/PUSH_IMAGE:TAG-disk
GitHubActions->>ghcr_io: podman push PUSH_REGISTRY/PUSH_IMAGE:TAG-disk
GitHubActions->>ghcr_io: podman tag NODE_IMAGE PUSH_REGISTRY/PUSH_IMAGE:latest-disk
GitHubActions->>ghcr_io: podman push PUSH_REGISTRY/PUSH_IMAGE:latest-disk
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
PUSH_REGISTRYenv var in the workflow actually includes both registry and repository (ghcr.io/alicefr/bink), which is a bit misleading; consider splitting this into separatePUSH_REGISTRYandPUSH_REPOSITORY(or similar) for clarity and to avoid confusion withREGISTRYin the Makefile. - The registry/repository string is now duplicated between the workflow (
PUSH_REGISTRY) and the Makefile (REGISTRYdefault); consider adding a small Makefile target to print the fully qualified image repo and let the workflow read it, so there is a single source of truth for image location.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `PUSH_REGISTRY` env var in the workflow actually includes both registry and repository (`ghcr.io/alicefr/bink`), which is a bit misleading; consider splitting this into separate `PUSH_REGISTRY` and `PUSH_REPOSITORY` (or similar) for clarity and to avoid confusion with `REGISTRY` in the Makefile.
- The registry/repository string is now duplicated between the workflow (`PUSH_REGISTRY`) and the Makefile (`REGISTRY` default); consider adding a small Makefile target to print the fully qualified image repo and let the workflow read it, so there is a single source of truth for image location.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The workflow's REGISTRY env var was overriding the Makefile's REGISTRY ?= default, causing images to be built with wrong names. Rename workflow env vars to PUSH_REGISTRY/PUSH_IMAGE, query the Makefile for actual built image names, and rename bink-node to node.
Summary by Sourcery
Resolve container image naming and registry conflicts between the CI workflow and the Fedora node image Makefile.
Bug Fixes:
Enhancements: